Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add example of how to prefetch ssr data #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

agungsb
Copy link

@agungsb agungsb commented Jun 10, 2017

I add an example on how to prefetch ssr data as described here.

I also add clientMiddleware.js and ApiClient.js so it's possible to make a call request inside redux actions (the source code is from this awesome boilerplate).

@agungsb agungsb closed this Jun 10, 2017
@agungsb agungsb deleted the feature/prefetch-ssr-data-example branch June 10, 2017 18:03
@agungsb agungsb restored the feature/prefetch-ssr-data-example branch June 10, 2017 18:03
@agungsb agungsb reopened this Jun 10, 2017
@ayroblu
Copy link
Owner

ayroblu commented Jun 19, 2017

Hey, I'm gonna be honest, there's ALOT of code here to do something really simple?
Just a quick summary:
Stylistic changes:

  • In the index.html where you lower the head element (why?)
  • DATA = {{WINDOW_DATA}} // this I like, because I think its the only way it can be done, though I would prefer something else, I've heard http2 push might also work??? Dunno, but I approve of this
  • I like const {Thing} = this.props // Perhaps I'm not consistent everywhere, but I try, so looks like you tried to impose the const { Thing } = this.props // where you add the extra space
  • I use leading commas, you don't
  • I use no semi colons, you don't // I'm not saying they're wrong, just its super weird cause you can see the seams where different opinions meet

Code:

  • You define a routes, why? React router v4 doesn't require routes defined, PLUS you redefine them in multiple places, which makes maintenance a bit of a pain
  • You add superagent and a new ApiClient file (I already have an api file right?) plus, you don't explain why you need superagent? fetch is great and you don't need any extra dependencies, it doesn't add anything to the demo
  • You don't redux right. Types go in the types, so that they're shared, rather than redefined
  • You add a middleware file just to handle all the code loading?

Okay, this is taking me too much time, you've made a lot of changes that I feel could simply be done with:

action.set({start: true})
fetch(url)
  .then(v=>action.set({page: v}))
  .catch(error=>action.set({error})

Can you justify your changes?

@thierryskoda
Copy link

Would love to have this feature well integrated 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants